[DO NOT MERGE] Code review by jairad26 · Pull Request #53 · hypermodeinc/modusGraph

jairad26 · 2025-01-03T19:43:11Z

No description provided.

Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.29.0 to 0.31.0. <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/golang/crypto/commit/b4f1988a35dee11ec3e05d6bf3e90b695fbd8909"><code>b4f1988</code></a> ssh: make the public key cache a 1-entry FIFO cache</li> <li><a href="https://github.com/golang/crypto/commit/7042ebcbe097f305ba3a93f9a22b4befa4b83d29"><code>7042ebc</code></a> openpgp/clearsign: just use rand.Reader in tests</li> <li><a href="https://github.com/golang/crypto/commit/3e90321ac7bcee3d924ed63ed3ad97be2079cb56"><code>3e90321</code></a> go.mod: update golang.org/x dependencies</li> <li><a href="https://github.com/golang/crypto/commit/8c4e668694ccbaa1be4785da7e7a40f2ef93152b"><code>8c4e668</code></a> x509roots/fallback: update bundle</li> <li>See full diff in <a href="https://github.com/golang/crypto/compare/v0.29.0...v0.31.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=golang.org/x/crypto&package-manager=go_modules&previous-version=0.29.0&new-version=0.31.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/hypermodeinc/modusDB/network/alerts). </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

…ort (#35)

This PR adds the initial issue and PR templates, a changelog, code of conduct, contributing guide, and security reporting instructions. We can enhance these over time.

**Description** Initialize Trunk for broader linting and security monitoring. Fix addressable issues. **Checklist** - [x] Code compiles correctly and linting passes locally

**Description** add readme to repo

**Description** Adds copyrights to all go files **Checklist** - [x] Code compiles correctly and linting passes locally - [ ] For all _code_ changes, an entry added to the `CHANGELOG.md` file describing and linking to this PR - [x] Tests added for new functionality, or regression tests for bug fixes added as applicable - [ ] For public APIs, new features, etc., PR on [docs repo](https://github.com/hypermodeinc/docs) staged and linked here Thank you for your contribution to modusDB!

**Description** Move GitHub Actions workflows to Warp runners for faster tests through larger machine access. **Checklist** - [X] Code compiles correctly and linting passes locally

**Description** This PR adds support for reverse edges via the readFrom json tag **Checklist** - [x] Code compiles correctly and linting passes locally - [x] For all _code_ changes, an entry added to the `CHANGELOG.md` file describing and linking to this PR - [x] Tests added for new functionality, or regression tests for bug fixes added as applicable - [ ] For public APIs, new features, etc., PR on [docs repo](https://github.com/hypermodeinc/docs) staged and linked here

**Description** refactoring packages for clearer code, remove duplications, etc **Checklist** - [x] Code compiles correctly and linting passes locally - [x] For all _code_ changes, an entry added to the `CHANGELOG.md` file describing and linking to this PR - [ ] Tests added for new functionality, or regression tests for bug fixes added as applicable - [ ] For public APIs, new features, etc., PR on [docs repo](https://github.com/hypermodeinc/docs) staged and linked here

**Description** more refactoring to simplify code in main modusdb package **Checklist** - [x] Code compiles correctly and linting passes locally

mangalaman93

I also do not understand the package structure having two levels of hierarchy with just one file in each package. This does not feel useful to me. packages are for dividing code for logically structuring it. I don't see that here.

This is a 10000 feet view of the PR. I will do another pass once some of these comments are addressed. This is a very long PR to review.

mangalaman93 · 2025-01-06T06:29:05Z

api/mutations/mutations.go

+	"github.com/hypermodeinc/modusdb/api/utils"
+)
+
+func HandleReverseEdge(jsonName string, value reflect.Type, nsId uint64, sch *schema.ParsedSchema,


we should move these to internal package if we want to have this package structure.

api/mutations/mutations.go

mangalaman93 · 2025-01-06T06:31:01Z

api/query_gen/dql_query.go

+	return fmt.Sprintf(ObjQuery, qf(), extraFields)
+}
+
+func FormatObjsQuery(typeName string, qf QueryFunc, paginationAndSorting string, extraFields string) string {


what is the benefit of passing a function qf instead of passing the return value here?

formatObjsQuery doesn't need queryfunc, its there to maintain code readability bc formatObjQuery does need it. it makes more sense if they both follow using qf

api/utils/reflect.go

api/utils/utils.go

mangalaman93 · 2025-01-06T06:34:34Z

api_mutation_helpers.go

+	return gid, nil
+}
+
+func applyDqlMutations(ctx context.Context, db *DB, dms []*dql.Mutation) error {


All the functions that do mutations and query or alter or any other Dgraph operation, that should use moduDB APIs or equivalent so that we can make changes to it easily.

…is specifically

**Description** On query of a type with a reverse edge, it modifies the type to remove all other predicates. The expected logic should only alter the schema if the type and predicate for the reverse edge does not exist. **Checklist** - [x] Code compiles correctly and linting passes locally - [x] For all _code_ changes, an entry added to the `CHANGELOG.md` file describing and linking to this PR - [x] Tests added for new functionality, or regression tests for bug fixes added as applicable - [ ] For public APIs, new features, etc., PR on [docs repo](https://github.com/hypermodeinc/docs) staged and linked here

.github/ISSUE_TEMPLATE/bug_report.md

mattjohnsonpint · 2025-01-08T05:53:09Z

.github/ISSUE_TEMPLATE/feature_request.md

+**Is your feature request related to a problem? Please describe.** A clear and concise description
+of what the problem is. Ex. I'm always frustrated when [...]
+
+**Describe the solution you'd like** A clear and concise description of what you want to happen.
+
+**Describe alternatives you've considered** A clear and concise description of any alternative
+solutions or features you've considered.
+
+**Additional context** Add any other context or screenshots about the feature request here.


Trunk merged the title and body of these paragraphs, because they were just bold text rather than headings. Use ## headings instead.

mattjohnsonpint · 2025-01-08T05:58:09Z

CONTRIBUTING.md

Consider adding a section about Trunk. Feel free to copy from the one I did in Modus.

mattjohnsonpint · 2025-01-08T05:58:25Z

.github/pull_request_template.md

This looks like it was copied from the Modus repo, but I've recently changed the text there. Please consider updating to match. (At minimum, update the headings to use ## markdown.)

.github/workflows/ci-go-lint.yaml

mattjohnsonpint · 2025-01-08T17:43:35Z

.trunk/configs/.markdownlint.json

Since this is in trunk's configs dir, you'll be fighting against VS Code expecting it in the root. Trunk will have this config, and VS Code will have the default config.

You can fix that by adding the following to .vscode/settings.json

"markdownlint.config": { "extends": ".trunk/configs/.markdownlint.json" },

@mattjohnsonpint is this true even if you don't install markdownlint in the editor? Trunk takes care of running it, so I don't think it's needed as a VS Code extension

mattjohnsonpint · 2025-01-08T17:47:29Z

CHANGELOG.md

+- feat: add readfrom json tag to support reverse edges
+  [#49](https://github.com/hypermodeinc/modusDB/pull/49)
+
+- chore: Refactoring package management #51 [#51](https://github.com/hypermodeinc/modusDB/pull/51)


Suggested change

- chore: Refactoring package management #51 [#51](https://github.com/hypermodeinc/modusDB/pull/51)

- chore: Refactoring package management [#51](https://github.com/hypermodeinc/modusDB/pull/51)

mattjohnsonpint · 2025-01-08T17:50:08Z

CONTRIBUTING.md

+
+If in doubt or contemplating a larger change, join the
+[Hypermode Discord](https://discord.hypermode.com) and start a discussion in the
+[#modus](https://discord.com/channels/1267579648657850441/1292948253796466730) channel.


Suggested change

[#modus](https://discord.com/channels/1267579648657850441/1292948253796466730) channel.

[#modusdb](https://discord.com/channels/1267579648657850441/1324490523725135872) channel.

mattjohnsonpint · 2025-01-08T17:55:48Z

README.md

+}
+
+func main() {
+  db, err := New(NewDefaultConfig("/tmp/modusdb"))


Suggested change

db, err := New(NewDefaultConfig("/tmp/modusdb"))

cfg := modusdb.NewDefaultConfig("/tmp/modusdb")

db, err := modusdb.New(cfg)

mattjohnsonpint · 2025-01-08T18:01:10Z

README.md

+  }
+  fmt.Println(user)
+
+  _, queriedUser, err := modusdb.Get[User](db, gid)


It's unclear what the _ return value is. If it's the gid, why does the API accept a gid and also return one? Could they differ somehow?

mattjohnsonpint · 2025-01-10T01:00:34Z

api.go

+	"github.com/hypermodeinc/modusdb/api/apiutils"
+	"github.com/hypermodeinc/modusdb/api/structreflect"
+)
+


Please add usage comments for at least these primary public functions. Otherwise, it's difficult to tell what the parameters and return values are supposed to be. Thanks.

mattjohnsonpint · 2025-01-10T01:10:15Z

live_test.go

These tests take too long. Please separate this and other long-running integration tests using build tags. Thanks.

mattjohnsonpint · 2025-01-10T01:16:50Z

api.go

+	if len(ns) > 1 {
+		return nil, nil, fmt.Errorf("only one namespace is allowed")
+	}
+	ctx, n, err := getDefaultNamespace(db, ns...)


Here and in the rest of the API methods, we are creating the ctx context object ourselves inside the getDefaultNamespace function. Instead, we should be allowing the user to pass context in as a parameter (usually the first parameter).

For example, say the user wants the query to timeout if it takes to long to run. The normal way would be to use context.WithTimeout and pass that context in to the query function. But we've not given them any way to do that.

... Also, it's unclear why there is Query as a global function that doesn't take a context but takes a db and a namespace, and db.Query which does take a context, but not a namespace.

IMHO, There should just be one way to query, not two.

mattjohnsonpint · 2025-01-13T16:57:12Z

db.go

+	return db.defaultNamespace.Mutate(ctx, ms)
 }

 func (db *DB) Load(ctx context.Context, schemaPath, dataPath string) error {


Let's just make this one LoadSchema(ctx, schemaPath), since we already have LoadData that does the same thing with dataPath.

Suggested change

func (db *DB) Load(ctx context.Context, schemaPath, dataPath string) error {

func (db *DB) Load(ctx context.Context, schemaPath, dataPath string) error {

mattjohnsonpint · 2025-01-13T16:59:14Z

db.go

 func (db *DB) DropData(ctx context.Context) error {
-	return db.gxy.DropData(ctx)
+	return db.defaultNamespace.DropData(ctx)
 }

 func (db *DB) AlterSchema(ctx context.Context, sch string) error {
-	return db.gxy.AlterSchema(ctx, sch)
+	return db.defaultNamespace.AlterSchema(ctx, sch)
 }

 func (db *DB) Query(ctx context.Context, q string) (*api.Response, error) {
-	return db.gxy.Query(ctx, q)
+	return db.defaultNamespace.Query(ctx, q)
 }

 func (db *DB) Mutate(ctx context.Context, ms []*api.Mutation) (map[string]uint64, error) {
-	return db.gxy.Mutate(ctx, ms)
+	return db.defaultNamespace.Mutate(ctx, ms)
 }


Since these are the "escape hatches" - they shouldn't be primary methods on the db object. Let's put them elsewhere, such as db.advanced.QueryDQL etc... (maybe a different name, but under some object)

mattjohnsonpint · 2025-01-13T17:10:04Z

api.go

+	db.mutex.Lock()
+	defer db.mutex.Unlock()


I think this should be RLock and RUnlock

mattjohnsonpint · 2025-01-13T17:12:15Z

api.go

+	"github.com/hypermodeinc/modusdb/api/structreflect"
+)
+
+func Create[T any](db *DB, object T, ns ...uint64) (uint64, T, error) {


This pattern will become a problem as we may need other options in the future. I suggest this use the functional options pattern instead, ie, WithNamespace(uint64) - which is more idiomatic for Go anyway.

mattjohnsonpint · 2025-01-13T17:51:35Z

api.go

+		return 0, object, err
+	}
+
+	return getByGid[T](ctx, n, gid)


I'm so/so on whether these create/upsert functions should always do a get at the end. The user could just do that themselves. And in the case of multiple inserts, they may just throw away the result so it's extraneous work that affects insert perf.

mattjohnsonpint · 2025-01-13T18:04:16Z

api/structreflect/structreflect.go

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package structreflect


This package and others we don't want the user calling should be nested under an internal package

**Description** This PR updates the nomenclature from db and namespace to engine and namespace to better represent common DB terminology it also removes the notion of opertaion on both the entities, all mutations, queries, and drop datas are conducted on namespaces. the engine is responsibile for drop all & maintaining namespaces & state **Checklist** - [x] Code compiles correctly and linting passes locally - [x] For all _code_ changes, an entry added to the `CHANGELOG.md` file describing and linking to this PR - [x] Tests added for new functionality, or regression tests for bug fixes added as applicable - [ ] For public APIs, new features, etc., PR on [docs repo](https://github.com/hypermodeinc/docs) staged and linked here

**Description** separate load and unit tests for faster workflow checks

This tells Trunk to ignore this issue raised by osv-scanner. Details of the issue are here: GHSA-9w9f-6mg8-jp7w It's safe to ignore the issue, as we do not consume the `bleve/http` pacakge.

**Description** Please explain the changes you made here. **Checklist** - [x] Code compiles correctly and linting passes locally - [x] For all _code_ changes, an entry added to the `CHANGELOG.md` file describing and linking to this PR - [x] Tests added for new functionality, or regression tests for bug fixes added as applicable - [ ] For public APIs, new features, etc., PR on [docs repo](https://github.com/hypermodeinc/docs) staged and linked here

jairad26 and others added 19 commits December 18, 2024 13:28

Jai/hyp 2765 get api (#28)

bb00fef

separate ci tests and lint (#30)

208821a

Create CODEOWNERS

571b799

add readFrom, index creation, and gid for apis (#31)

0420655

Jai/hyp 2771 benchmark modusdb (#32)

a8aee3e

upgrade dependencies in go.mod (#24)

2e3b106

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

add delete api (#33)

3e824e2

add nested mutation, getting nested values, upsert, nested types supp…

1116c7c

…ort (#35)

support unstructured data in modusdb (#38)

84b781b

chore: repo setup (#43)

f7007a4

This PR adds the initial issue and PR templates, a changelog, code of conduct, contributing guide, and security reporting instructions. We can enhance these over time.

chore: create LICENSE (#42)

fff8aaa

chore: add trunk (#44)

e676bc6

**Description** Initialize Trunk for broader linting and security monitoring. Fix addressable issues. **Checklist** - [x] Code compiles correctly and linting passes locally

chore: add readme (#46)

a157525

**Description** add readme to repo

Update trunk.yaml

77effe0

chore: move to Warp runners for GitHub Actions (#50)

2e7ac26

**Description** Move GitHub Actions workflows to Warp runners for faster tests through larger machine access. **Checklist** - [X] Code compiles correctly and linting passes locally

jairad26 requested review from mangalaman93 and mattjohnsonpint January 3, 2025 19:43

chore: move reflection logic to utils package (#54)

7abdc72

**Description** more refactoring to simplify code in main modusdb package **Checklist** - [x] Code compiles correctly and linting passes locally

mangalaman93 suggested changes Jan 6, 2025

View reviewed changes

jairad26 added 5 commits January 6, 2025 00:19

revs

48b27de

.

205eaf7

rename internal to api to reflect the functions inside are for the ap…

f24f940

…is specifically

Merge branch 'main' into code-review

b9226a5

mattjohnsonpint reviewed Jan 10, 2025

View reviewed changes

mattjohnsonpint reviewed Jan 13, 2025

View reviewed changes

jairad26 and others added 5 commits January 15, 2025 08:30

chore: separate load and unit tests (#59)

9b6934e

**Description** separate load and unit tests for faster workflow checks

chore: Ignore blev warning (#60)

b63cc2a

This tells Trunk to ignore this issue raised by osv-scanner. Details of the issue are here: GHSA-9w9f-6mg8-jp7w It's safe to ignore the issue, as we do not consume the `bleve/http` pacakge.

Merge branch 'main' into code-review

69abfd7

ryanfoxtyler closed this Apr 25, 2025

	- chore: Refactoring package management #51 [#51](https://github.com/hypermodeinc/modusDB/pull/51)
	- chore: Refactoring package management [#51](https://github.com/hypermodeinc/modusDB/pull/51)

	[#modus](https://discord.com/channels/1267579648657850441/1292948253796466730) channel.
	[#modusdb](https://discord.com/channels/1267579648657850441/1324490523725135872) channel.

	db, err := New(NewDefaultConfig("/tmp/modusdb"))
	cfg := modusdb.NewDefaultConfig("/tmp/modusdb")
	db, err := modusdb.New(cfg)

	func (db *DB) Load(ctx context.Context, schemaPath, dataPath string) error {
	func (db *DB) Load(ctx context.Context, schemaPath, dataPath string) error {

Conversation

jairad26 commented Jan 3, 2025

Uh oh!

mangalaman93 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattjohnsonpint Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

mattjohnsonpint Jan 10, 2025 •

edited

Loading